New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add support for Symfony 7 #28
Add support for Symfony 7 #28
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added some feedback on specific things to clarify.
$container->setAlias('ambta_doctrine_annotation_reader','annotations.reader'); | ||
// Symfony 1-3 | ||
if (Kernel::MAJOR_VERSION < 5) { | ||
throw new \RuntimeException('doctrineencryptbundle/doctrine-encrypt-bundle expects symfony-version >= 4.1!'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This message should be updated.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this necessary? The composer.json
will prevent installing with old versions of Symfony.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not really, more of a sanity check.
} elseif (Kernel::MAJOR_VERSION < 7) { | ||
// PHP 7.x (no attributes) | ||
if (PHP_VERSION_ID < 80000) { | ||
$loader->load('services_subscriber_sf4_php7.yml'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps we should rename these files to
services_subscriber_with_annotations.yml
services_subscriber_with_annotations_and_attributes.yml
service_listeners_with_attributes.yml
.github/workflows/ci.yml
Outdated
# Disable lowest for now | ||
# include: | ||
# # testing lowest PHP version with lowest dependencies | ||
# - php-version: '7.2.5' | ||
# dependency-versions: 'lowest' | ||
# composer-options: '--prefer-lowest' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I could not get this to work, and perhaps we should also not support this any longer?
I don't mind supporting php 7.2 as long as it uses the latest versions it can (since sf 5.4 support it) but supporting it with the lowest versions possible of the supporting libraries is rather annoying.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Have you tried adding it back again after dropping support for symfony 4? Otherwise you might remove dead code
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I re-added it in 0a1f3c9.
I had to bump the minimum-requirement of doctrine/doctrine-bundle from 2.0.0 to 2.0.8 for the root-project and for the sf5.4-demo-project I updated the minimum-requirement of symfony/flex to 1.21.
To have phpunit run succesfully, I had to signal phpunit-bridge to use phpunit 7 for that specific job.
Good of you to catch it, since this way we can guarantee our minimum requirements actually work. 👍
Nice work! What else needs to be done to get this ready for merging? I'm not sure that a new major version is necessary, when the PS. This will close #8 |
I think the only thing needed is a little cleanup.
Rethinking this, the only possible fixes I would implement for symfony 4.x-versions would be security-fixes and that would only ever be a patch version. So my current stance would be that a minor-version would be fine. I'll try to take some time next week to do the cleanup. |
Any updates here @Zombaya or do you need support? This is the only lib that blocks my symfony 7 update 🙃 |
Squashed commits ---------------- * 28c5acf 2023-12-09 Zombaya Allow symfony 7.x in root-composer-json and add demo project for symfony 7 * 47526a7 2023-12-09 Zombaya Add demo-project using symfony 7.x * e93cce7 2023-12-09 Zombaya Use path of phpunit-bridge for symfony 4.x * 2649be2 2023-12-09 Zombaya Rework service-definitions and split it up in multiple files so we support sf 4.x-7.x * d42637b 2023-12-09 Zombaya Removed symfony 4.4 demo * a05da56 2023-12-09 Zombaya Add demo-project for sf 5.4 * d185546 2023-12-09 Zombaya Replace sf 4.4 by 5.4 in CI * a05b686 2023-12-09 Zombaya Drop support for sf 4.* in composer.json * 14362ff 2023-12-09 Zombaya Allow halite 4.6 in sf 5.4-demo * 388c358 2023-12-09 Zombaya Use seperate config for php 8 and 7 in sf 5.4 demo * 507e9b4 2023-12-09 Zombaya Drop support for sf 4 in DoctrineEncryptExtension as well * 9f23159 2023-12-09 Zombaya Move dev-requirements and allow more versions of phpunit-bridge * a4e79da 2023-12-09 Zombaya Never use phpunit directly - only use the bridge * 35add17 2023-12-09 Zombaya Ignore tests in phpstan for now * 0c8d2e0 2023-12-09 Zombaya Use simple-php-unit for root-project * aff5a52 2023-12-09 Zombaya Stop forcing phpunit version for phpunit-bridge in 5.4-demo * 3172c8a 2023-12-09 Zombaya Remove forced version for php 7.2 for composer * b74e4fb 2023-12-09 Zombaya Require recent version of phpunit-bridge for root-project * 53260f6 2023-12-09 Zombaya Remove lowest for now * d414bb8 2023-12-09 Zombaya Add CI-run for demo 7.x * f58810f 2023-12-09 Zombaya Restrict CI for sf7 to php 8.2 * a1189b2 2024-02-26 Zombaya Small cleanup after review * 7ce2724 2024-02-26 Zombaya Small fix for missing rename of file
@r3hp1c , as far as I'm concerned, this is ready to be merged in and released. I squashed everything into a single commit. This will drop support for symfony 4.x and up the minimal requirement to symfony 5.4 but I think that is fair since older versions are also no longer supported by symfony itself. @core23 , thanks for wanting to help, but the only thing missing was a motivation to work after hours 🙂 Hopefully your upgrade to symfony 7 will run smoothly after this is released. |
composer.json
Outdated
"defuse/php-encryption": "^2.1", | ||
"doctrine/cache": "^1.11", | ||
"phpstan/phpstan": "^1.4", | ||
"jetbrains/phpstorm-attributes": "^1.0", | ||
"phpcompatibility/php-compatibility": "^9.3", | ||
"symfony/phpunit-bridge": "^6.0" | ||
"symfony/phpunit-bridge": "^6.4|^7.0" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you can drop symfony/phpunit-bridge 6 completly. It is mostly backward compatible
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I checked the docs and you're right, we can drop the support for 6.4, especially since they are in the dev-requirements, which do not affect users of our library.
From the docs:
The PHPUnit bridge is designed to work with all maintained versions of Symfony components,
even across different major versions of them.
You should always use its very latest stable major version to get the most accurate deprecation report.
@Zombaya I am happy going forward to keep support the same as Symfony itself. |
From the documentation: > The PHPUnit bridge is designed to work with all maintained versions of Symfony components, > even across different major versions of them. > You should always use its very latest stable major version to get the most accurate deprecation report.
Changes ======= * Set variable to use correct phpunit to run on lowest See symfony/symfony#52844 * Up the minimum-requirement for symfony/flex for demo/sf5.4 to 1.21 * Update minimal version of doctrine/doctrine-bundle to allow installing with prefer-lowest on sf 5.4
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we're good to go now.
I added some code which should resolve the latest remarks by @core23.
.github/workflows/ci.yml
Outdated
# Disable lowest for now | ||
# include: | ||
# # testing lowest PHP version with lowest dependencies | ||
# - php-version: '7.2.5' | ||
# dependency-versions: 'lowest' | ||
# composer-options: '--prefer-lowest' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I re-added it in 0a1f3c9.
I had to bump the minimum-requirement of doctrine/doctrine-bundle from 2.0.0 to 2.0.8 for the root-project and for the sf5.4-demo-project I updated the minimum-requirement of symfony/flex to 1.21.
To have phpunit run succesfully, I had to signal phpunit-bridge to use phpunit 7 for that specific job.
Good of you to catch it, since this way we can guarantee our minimum requirements actually work. 👍
composer.json
Outdated
"defuse/php-encryption": "^2.1", | ||
"doctrine/cache": "^1.11", | ||
"phpstan/phpstan": "^1.4", | ||
"jetbrains/phpstorm-attributes": "^1.0", | ||
"phpcompatibility/php-compatibility": "^9.3", | ||
"symfony/phpunit-bridge": "^6.0" | ||
"symfony/phpunit-bridge": "^6.4|^7.0" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I checked the docs and you're right, we can drop the support for 6.4, especially since they are in the dev-requirements, which do not affect users of our library.
From the docs:
The PHPUnit bridge is designed to work with all maintained versions of Symfony components,
even across different major versions of them.
You should always use its very latest stable major version to get the most accurate deprecation report.
First of all, thanks for all contributions made already! This PR seems to be ready for a merge and release. Are there any open issues we could help with to get this PR merged? |
} | ||
|
||
// Symfony 5-6 | ||
if (Kernel::MAJOR_VERSION < 7) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just tested it with one of my projects where I'm using symfony 6.4, but doctrine-extension 7.
Sadly this check does not work as expected and might need some adjustments for that case
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As said in #30 (comment), I think this issue should be resolved by sticking to either 6.4 or 7.0, but not both at the same time.
On the hackaton of symfonycon brussels 2023, I spend the day trying to add support for symfony 7, which I believe has succeeded!
I do think I need to clean things up a bit and squash a lot of commits to get a more clean history, but I wanted to create this draft PR to let you know about the work I did.
Breaking changes
I was hitting some issues with getting the unittests to run on symfony 4.4. As official support for symfony 4.4 has ended, I found this to be a good time to also drop support for symfony 4.4 and only support symfony 5.4+.
Perhaps we can create a new branch
5.x
in case we need to push changes or add deprecation triggers to the current version of the project, release a6.0.0
-version and develop this in main.Implementation
I rewrote the service-definition a bit to split up the services based on which symfony version is being used, since starting from symfony 7, the annotation-driver is no longer present.
Starting from symfony 7, you also need to register doctrine-event-listeners instead of a doctrine-event-subscriber. As I wanted to just add support for symfony 7, I just registered the subscriber as a listener, without changing the implementation.
Future implementation
I would like to rewrite the service-definition so that we would pull the detection of which fields of which classes to encrypt outside of the listener/subscriber so that we can define proper listeners which only listen for events on classes with the encrypted-attribute.
As I just wanted it to work on symfony 7, I did not implement this yet.
Work to be done